-
Notifications
You must be signed in to change notification settings - Fork 708
feat: Add .pyroscope.yaml with source code mapping support
#4606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1a8e75f to
fa7f045
Compare
| type: string | ||
| title: rootPath | ||
| description: the root path where the project lives inside the repository | ||
| functionName: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like currently logic is: use localPath if go, use functionName if java. May lead to confusion down the road if we start seeing java profiles with the file paths properly set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I guess the profiling data you send needs to match with the .pyroscope.yaml you set. I do think this makes sense and is both in customer control.
d5ab32d to
85d78f1
Compare
- Support golang.org/toolchain paths - Use mappings if there is no version identified
e34c558 to
7d5ab4e
Compare
.pyroscope.yaml with source code mapping support
bryanhuhta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts for the future, but I like where this is at right now.
jake-kramer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
| } | ||
|
|
||
| // Validate checks if a source configuration is valid | ||
| func (m *Source) Validate() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be helpful error message to surface if the supplied source is not supported (i.e. not github or local)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not fully sure, if I am understanding this comment: There are two parts:
Not surfacing the error: I agree nothing surfaces this right now to a user, I think we will need first CLI support like profilescli pyroscope-yaml validate, but it should also be possible to surface them in Drilldown, when function details is used.
"if the supplied source is not supported (i.e. not github or local)": This will trigger the error in Line 136. Not too sure what else we could do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the "supplied source is not supported" I meant specifically this scenario:
mappings:
- function_name:
- prefix: org/example/rideshare
language: java
source:
local:
path: src
- function_name:
- prefix: org/example/foo
language: java
source:
gitlab:
path: srcwill return the error "no source type supplied, you need to supply exactly one source type" which does not let the user know that gitlab is an unsupported source
| return | ||
| } | ||
| sp.SetTag("config.url", file.URL) | ||
| sp.SetTag("config", file.Content) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this too large to tag in span?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not aware of any particular limits, infact this might be the only way for us to see those files, as we don't really have access to them if they are in a private repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this max_attribute_bytes config which defaults to 2048 is related?
| sp.SetTag("config.url", file.URL) | ||
| sp.SetTag("config", file.Content) | ||
|
|
||
| cfg, err := config.ParsePyroscopeConfig([]byte(file.Content)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I still think it would be useful to load up this config when the frontend loads up and WARN any errors just as a useful sanity check. OK to instead rely on config tooling I'm working on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think this is possible, we need an end user to supply us an encrypted token and the owner/repo/root-path information before we can fetch a config.
| // fetchRepoFile fetches the file content from the configured repository. | ||
| func (arg FileFinder) fetchRepoFile(ctx context.Context, path, ref string) (*vcsv1.GetFileResponse, error) { | ||
| if arg.rootPath != "" { | ||
| path = filepath.Join(arg.rootPath, path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: There's sort of a rough edge here where this root path is defined in the UI at repo level and then the mapping config can contain local paths that are function-level root paths. I can see as a user being confused playing with these two configs.
Maybe at some point a helper tooltip in the UI can clarify how these configs interact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I agree, in order to make the UX workable, we need to show the parsed config in context to what else is configured. I don't think though this will be a good investment of time just yet.
At least of the paths in .pyroscope.yaml are relative to that file, and the only location we are reading .pyroscope.yaml from is the "rootPath".
Also update the script for `.pyroscope.yaml` (was missed in #4606)
This PR adds support for
.pyroscope.yamlconfiguration files for custom source code mappings and adds logic to use those mappings to find Go and Java codeConfiguration Example (also in the PR):
This will map a special path prefix called
$GOROOTto a particular go version. This signals to the backend the go version used to compile this should be kept update to date with the version of Pyroscope used to build.